Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Updates download-page dropdowns to pass Reviewed by Cursor Bugbot for commit 92c3f10. Bugbot is set up for automated code reviews on this repo. Configure here. |
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8821 +/- ##
==========================================
- Coverage 73.90% 73.87% -0.03%
==========================================
Files 105 105
Lines 8889 8883 -6
Branches 326 326
==========================================
- Hits 6569 6562 -7
- Misses 2319 2320 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
Refactors the shared Select UI component to remove its internal “dual state” behavior and shift to an externally controlled value API, then updates downstream call sites (site download dropdowns + Storybook) to match.
Changes:
- Replace
defaultValuewithvalueinSelectPropsand remove internal state syncing fromSelect. - Update Storybook stories to keep
valuein sync viauseArgs. - Update Downloads “Release” dropdown components to pass
valueinstead ofdefaultValue, and adjust NoScript/Stateless select typings/wiring.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/Common/Select/index.tsx | Removes internal state and changes public API from defaultValue → value. |
| packages/ui-components/src/Common/Select/index.stories.tsx | Updates stories to use value and keeps Storybook args controlled via useArgs. |
| packages/ui-components/src/Common/Select/StatelessSelect/index.tsx | Adjusts props typing to reintroduce defaultValue semantics for StatelessSelect. |
| packages/ui-components/src/Common/Select/NoScriptSelect/index.tsx | Threads defaultValue into JS Select as value and into noscript select as defaultValue. |
| apps/site/components/Downloads/Release/PlatformDropdown.tsx | Migrates Select usage to value. |
| apps/site/components/Downloads/Release/PackageManagerDropdown.tsx | Migrates Select usage to value. |
| apps/site/components/Downloads/Release/OperatingSystemDropdown.tsx | Migrates Select usage to value. |
| apps/site/components/Downloads/Release/InstallationMethodDropdown.tsx | Migrates Select usage to value. |
Comments suppressed due to low confidence (2)
packages/ui-components/src/Common/Select/index.tsx:115
handleChangeno longer updates any internal value. Because the trigger rendering (currentItem) is derived solely from thevalueprop, the Select won't visually update when used without a controlledvalue(or when callers still pass the now-ignoreddefaultValueprop). This is likely to break existing uncontrolled usage (and the current Select unit tests that passdefaultValue). Consider either (a) makingvaluerequired and documenting Select as controlled-only, or (b) restoring an uncontrolled path (e.g., keep internal state only whenvalueis undefined / support adefaultValueprop and avoid overridingSelectPrimitive.Valuecontent in uncontrolled mode).
const handleChange = (value: T) => {
if (typeof onChange === 'function') {
onChange(value);
}
};
packages/ui-components/src/Common/Select/index.tsx:35
- This change removes the public
defaultValueprop fromSelectPropsand replaces it withvalue, which is a breaking API change for any consumers using Select uncontrolled/with an initial default. If this package is consumed outside this repo, consider keepingdefaultValueas a deprecated alias (or doing a major version bump + migration) to avoid silent runtime regressions (e.g.,defaultValuecurrently becomes an ignored prop).
export type SelectProps<T extends string> = {
values: Array<SelectGroup<T>> | Array<T> | Array<SelectValue<T>>;
value?: T;
placeholder?: string;
label?: string;
inline?: boolean;
onChange?: (value: T) => void;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const WithNoScriptSelect = <T extends string>({ | ||
| as, | ||
| defaultValue, | ||
| ...props | ||
| }: StatelessSelectProps<T>) => { | ||
| const id = useId(); | ||
| const selectId = `select-${id.replace(/[^a-zA-Z0-9]/g, '')}`; | ||
|
|
||
| return ( | ||
| <> | ||
| <Select {...props} fallbackClass={selectId} /> | ||
| <Select {...props} value={defaultValue} fallbackClass={selectId} /> | ||
| <noscript> | ||
| <style>{`.${selectId} { display: none!important; }`}</style> | ||
| <StatelessSelect {...props} as={as} /> | ||
| <StatelessSelect {...props} defaultValue={defaultValue} as={as} /> | ||
| </noscript> |
There was a problem hiding this comment.
WithNoScriptSelect accepts a prop named defaultValue, but it is now passed through to the interactive <Select> as a controlled value. If a consumer provides a static defaultValue without updating it on onChange, the selection will appear “stuck” (no internal updates). To avoid this API trap, consider renaming this prop to value (and/or adding a wrapper state for the JS-enabled Select while keeping defaultValue semantics for the noscript version).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a1bd464. Configure here.
araujogui
force-pushed
the
refactor/select-defaultvalue
branch
from
a1bd464 to
208a19d
Compare
|
I think this reduces complexity and is a better naming choice. Applying this prop naming to It would also be worth checking |
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
@araujogui can you please verify all selects on all pages including with mobile and desktop viewport and with JavaScript enabled and disabled they still work and behave as expected?
araujogui
force-pushed
the
refactor/select-defaultvalue
branch
from
208a19d to
6a31a1d
Compare
araujogui
force-pushed
the
refactor/select-defaultvalue
branch
from
6a31a1d to
1480df2
Compare
I thought we could use |
Description
Refactor selects with dual controlled state to just one
Validation
Try changing selects
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.